Implement edmTypeInfo - #50842
Conversation
|
cms-bot internal usage |
|
type ngt |
|
please test |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-50842/49190 |
|
A new Pull Request was created by @fwyzard for master. It involves the following packages:
@Dr15Jones, @makortel, @smuzaffar can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
-1 Failed Tests: RelVals-INPUT Failed RelVals-INPUT
Comparison SummarySummary:
|
|
please test The failure is unrelated. |
|
+1 Size: This PR adds an extra 16KB to repository Comparison SummarySummary:
|
|
Milestone for this pull request has been moved to CMSSW_20_0_X. Please open a backport if it should also go in to CMSSW_17_0_X. |
| EXPECTED='`portabletest::TestHostCollection` resolves to `PortableHostCollection<portabletest::TestSoALayout<128,false> >` | ||
| with friendly class name `128falseportabletestTestSoALayoutPortableHostCollection` | ||
| with type info `22PortableHostCollectionIN12portabletest13TestSoALayoutILm128ELb0EEEE`' | ||
| compare "${CMD}" "${EXPECTED}" |
There was a problem hiding this comment.
These tests introduce a dependence outside of the framework packages that I'd much prefer to avoid. Best would be to add new test types to FWCore/Reflection/test/stubs.
Tests for portabletest types could be done in DataFormats/PortableTestObjects/test (if they would be useful).
There was a problem hiding this comment.
The tests are supposed to test the handling of ALPAKA_ACCELERATOR_NAMESPACE, and I assume you would rather not have dependencies on HeterogeneousCore/AlpakaCore and DataFormats/Portable under WCore/Reflection/test/stubs.
Is a dependency on DataFormats/TestObjects acceptable?
There was a problem hiding this comment.
The tests are supposed to test the handling of
ALPAKA_ACCELERATOR_NAMESPACE, and I assume you would rather not have dependencies onHeterogeneousCore/AlpakaCoreandDataFormats/PortableunderFWCore/Reflection/test/stubs.
Correct, those dependencies should be avoided.
Is a dependency on
DataFormats/TestObjectsacceptable?
It would be better, but at the package level all DataFormats depend transitively on FWCore/Reflection, so having FWCore/Reflection/test depend on a DataFormats package smells (although it would technically work).
But what could be achieved in DataFormats/TestObjects that could not be done in FWCore/Reflection/test/stubs?
There was a problem hiding this comment.
But what could be achieved in
DataFormats/TestObjectsthat could not be done inFWCore/Reflection/test/stubs?
I simply wanted to use some existing data format instead of creating new ones for the test.
What about DataFormats/StdDictionaries or DataFormats/Common ?
There was a problem hiding this comment.
What about
DataFormats/StdDictionariesorDataFormats/Common?
Dependency-wise those are not better than DataFormats/TestObjects.
If you just want something, I'd suggest edmtest::reflection::IntObject which is already defined in FWCore/Reflection/test/stubs.
There was a problem hiding this comment.
OK, I've changed the local test to use that, and added a test for alpaka types under DataFormats/PortableTestObjects.
c795c37 to
88eb3eb
Compare
Implement an utility to resolve a C++ type name to its actual definition, taking into account type aliases and the use of ALPAKA_ACCELERATOR_NAMESPACE, and print the full c++ name, the friendly class name, and the mangled name.
88eb3eb to
d8013dd
Compare
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-50842/50214
|
|
Pull request #50842 was updated. @Dr15Jones, @cmsbuild, @fwyzard, @makortel, @smuzaffar can you please check and sign again. |
|
please test |
|
+1 Size: This PR adds an extra 24KB to repository Comparison SummarySummary:
|
|
Comparison differences are related to #47071 |
|
+core |
|
+heterogeneous |
|
This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @ftenchini, @mandrenguyen, @sextonkennedy (and backports should be raised in the release meeting by the corresponding L2) |
|
+1 |
|
@fwyzard , the newly added unit test is failing for IBs without CUDA (e.g. our GCC 15 , GCC16 IBs) |
|
Ah... I will have a look, but I'm on holiday until the end of next week, and I don't know when I'll be able to. If it's a problem you can revert it for the moment. |
|
@smuzaffar #51580 should fix the test for builds without CUDA or ROCm. |
5 participants
PR description:
Implement an utility to resolve a C++ type name to its actual definition, taking into account type aliases and the use of
ALPAKA_ACCELERATOR_NAMESPACE, and print the full c++ name, the friendly class name, and the mangled name.PR validation:
Validate the new script with a dedicated unit test.